[Backport 2.x] [Bugfix] Fix TieredSpilloverCache stats not adding correctly when sha… #16790
Conversation
…rds are closed (opensearch-project#16560) * added draft tests for tsc stats holder Signed-off-by: Peter Alfonsi <[email protected]> * first draft tsc stats bugfix Signed-off-by: Peter Alfonsi <[email protected]> * Complete tests Signed-off-by: Peter Alfonsi <[email protected]> * Cleanup Signed-off-by: Peter Alfonsi <[email protected]> * Integrate fix with TSC Signed-off-by: Peter Alfonsi <[email protected]> * Add IT Signed-off-by: Peter Alfonsi <[email protected]> * Refactor cache package names in TSC module to match with server Signed-off-by: Peter Alfonsi <[email protected]> * changelog Signed-off-by: Peter Alfonsi <[email protected]> * Revert "Refactor cache package names in TSC module to match with server" This reverts commit 3b15a7a. Signed-off-by: Peter Alfonsi <[email protected]> * Addressed Sagar's comments Signed-off-by: Peter Alfonsi <[email protected]> * More package fixes Signed-off-by: Peter Alfonsi <[email protected]> * Addressed andross's comments Signed-off-by: Peter Alfonsi <[email protected]> --------- Signed-off-by: Peter Alfonsi <[email protected]> Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]> (cherry picked from commit c82cd2e)
peteralfonsi
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #16790 +/- ##
============================================
- Coverage 71.88% 71.82% -0.07%
+ Complexity 65506 65435 -71
============================================
Files 5314 5314
Lines 305364 305369 +5
Branches 44501 44501
============================================
- Hits 219522 219320 -202
- Misses 67535 67744 +209
+ Partials 18307 18305 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @peteralfonsi for raising this backport PR manually. Overall looks good, except for some of the seemingly unnecessary access modifiers. Can you fix that please, for main as well before merging this PR?
| return statsRoot; | ||
| } | ||
|
|
||
| /** | ||
| * Nodes that make up the tree in the stats holder. | ||
| */ | ||
| protected static class Node { | ||
| public static class Node { | ||
| private final String dimensionValue; | ||
| // Map from dimensionValue to the DimensionNode for that dimension value. | ||
| final Map<String, Node> children; |
There was a problem hiding this comment.
nit: Should this be private now since the getter is public? I am assuming we don't directly initialize children outside of the constructor?
There was a problem hiding this comment.
It can't be private since ImmutableCacheStatsHolder uses it during construction. But I can make it protected again. After reworking the TSC stats holder tests, it doesn't need to be public.
| Node getStatsRoot() { | ||
| public Node getStatsRoot() { | ||
| return statsRoot; | ||
| } |
There was a problem hiding this comment.
nit: Why does this need to be public? I don't any other usage throughout this PR
Signed-off-by: Peter Alfonsi <[email protected]>
|
@jainankitk The access changes were made because the TSC stats holder and original stats holder are in different packages, and testing was a problem without them. But, I was able to rework the tests to use the ImmutableCacheStatsHolder to read any necessary values, so I've reverted the access modifiers to the way they were before this PR. I'll raise the corresponding PR for main once we're happy with this version of the changes. |
Backport c82cd2e from #16560.
Opened a manual backport as the earlier one was getting stuck and I couldn't fix merge conficts without write permissions. I've closed the other one.